Thanks for this! I mentioned this on Zulip but wanted to echo here too -- I agree with the direction of this change and I think it would be good to have concrete error types for wit-parser. Implementation-wise, some thoughts from me are:

• If you're willing I think it'd be good to split this up and have pieces land incrementally. For example the changes to LexError and avoiding using anyhow in lex.rs is relatively separable. The end-goal could be to remove the anyhow dependency from this crate, but that wouldn't have to necessarily happen in one go (e.g. sections of APIs could be updated at-a-time).
• One benefit of anyhow::Error is its performance -- it's a pointer-large an non-nullable meaning that it can often fit into various niches (e.g. anyhow::Result<()> is a pointer large). While wit-parser isn't the most performance-sensitive of things I'd still be slightly worried about inflating the error sizes drastically (e.g. to dozens-of-pointers) which is unfortunately easy once there's lots of context added. This is solvable but would remove the pub enum WitError for example and would switch that to pub struct WitError with some sort of accessor like pub enum WitErrorKind(or something like that). We don't have any benchmarks forwit-parser` right now, though, so measuring the impact here is unfortunately not easy.
• Out of curiosity, do you need fine-grained rationale/context for all errors? Or are you mostly interested in just a Span for all errors? (or some sort of error-location) If it's just the latter I think we might be able to move in the direction of a concrete error without needing too much context information, but if the former that's a different possible design here.
• For Span-vs-rendered-string, personally I'd prefer to stick to using Span since that's what the rest of the API has, but you mention that it's unwieldy to turn that into a rendered location string. Can you elaborate more on that and how you feel that having a rendered string in the error is more appropriate?

Overall I'd say let's figure out the final design of the error types in play here, and once that's sorted out figure out how best to land this (e.g. all-at-once vs incrementally) -- does that sound ok?

Thank you for a very detailed response @alexcrichton !

If you're willing I think it'd be good to split this up and have pieces land incrementally. For example the changes to LexError and avoiding using anyhow in lex.rs is relatively separable. The end-goal could be to remove the anyhow dependency from this crate, but that wouldn't have to necessarily happen in one go (e.g. sections of APIs could be updated at-a-time).

Totally agreed. I'll try to merge some non-controversial pieces first.

One benefit of anyhow::Error is its performance -- it's a pointer-large an non-nullable meaning that it can often fit into various niches (e.g. anyhow::Result<()> is a pointer large). While wit-parser isn't the most performance-sensitive of things I'd still be slightly worried about inflating the error sizes drastically (e.g. to dozens-of-pointers) which is unfortunately easy once there's lots of context added. This is solvable but would remove the pub enum WitError for example and would switch that to pub struct WitError with some sort of accessor like pub enum WitErrorKind(or something like that). We don't have any benchmarks forwit-parser` right now, though, so measuring the impact here is unfortunately not easy.

To clarify - we'd just make WitError hold a box to WitErrorKind enum, which would then contain richer metadata (if needed)? ie just store error metadata on a heap instead. Or did you have something else in mind?

Out of curiosity, do you need fine-grained rationale/context for all errors? Or are you mostly interested in just a Span for all errors? (or some sort of error-location) If it's just the latter I think we might be able to move in the direction of a concrete error without needing too much context information, but if the former that's a different possible design here.

For a very basic LSP we don't need it, but I think it would be a good idea to be more fine-grained than just span+message. It's the difference between the consumers (the LSP, or even the CLI) knowing just what to highlight and what to say, vs these tools actually understanding what the error is.

Typed variants unlock richer things - DuplicatePackage naturally maps to multiple spans, and similarly PackageCycle can expose the cycle chain. In the future we can carry suggestions for fixes or other notes, and that would integrate really well with something like Ariadne (which would give us very informative rustc-like errors).

For Span-vs-rendered-string, personally I'd prefer to stick to using Span since that's what the rest of the API has, but you mention that it's unwieldy to turn that into a rendered location string. Can you elaborate more on that and how you feel that having a rendered string in the error is more appropriate?

I agree that the Span is the right internal primitive, but it raises the question for the public API - the consumer needs to know which source map a given Span is valid in.

• for SourceMap::parse() errors it's the map we just parsed
• for push_groups() errors it's resolve.source_map

This is manageable but leaks internal architecture to callers. In case of DuplicatePackage and PackageCycle I took a shortcut and just pre-rendered the messages because geting the right source map wasn't straightforward.

An alternative is for errors to carry already-resolved locations. Not pre-rendered strings, but structured (file_path, start_byte, end_byte) tuples resolved at the point where the source map is available (note - the byte offsets here would relative to the files themselves). The consumer then has everything they need without touching a source map, and can convert that easily to line/col.

I'm happy to keep exposing global spans in the API, but for that I might need to modify how package cycles are computed - from my understanding cycles are detected during toposort before source maps are merged, which is why I did eager resolution in my PR. I think if we merge the source maps before toposort that would work.